Skip to content

Mark stored procedure parameters without defaults as required in OpenAPI - #3687

Merged
souvikghosh04 merged 20 commits into
mainfrom
copilot/fix-openapi-required-params
Aug 3, 2026
Merged

Mark stored procedure parameters without defaults as required in OpenAPI#3687
souvikghosh04 merged 20 commits into
mainfrom
copilot/fix-openapi-required-params

Conversation

Copilot AI commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Why make this change?

  • Closes [Bug]: Required params not marked in OpenAPI #3498. The generated OpenAPI document for stored procedure entities omitted the required array on the {entity}_sp_request schema, so parameters that must be supplied (e.g. firstName, lastName) were not flagged as required.

What is this change?

  • CreateSpRequestComponentSchema (OpenApiDocumentor.cs): a parameter is now marked required when it is explicitly flagged in config or has no default value. Previously only an explicit config required: true populated the array, which is almost never set, so the array was always empty.
    • Mirrors the existing IsRequestBodyRequired logic (!HasConfigDefault), keeping body-level and property-level required semantics consistent. An explicit config required: false is still honored.
if (def.Required ?? !def.HasConfigDefault)
{
    required.Add(parameter);
}

How was this tested?

  • Integration Tests
  • Unit Tests

Extended StoredProcedureGeneration.ValidateRequestBodyContents to assert the request body schema's required set contains the expected parameters. Verified the assertion fails when the fix is reverted.

Sample Request(s)

GET /api/openapi — request body schema component for a stored procedure with parameters title, publisher_name (neither having a default):

"sp1_sp_request": {
  "type": "object",
  "properties": {
    "title": { "type": "string" },
    "publisher_name": { "type": "string" }
  },
  "required": [ "title", "publisher_name" ]
}

Copilot AI changed the title [WIP] Fix required params not marked in OpenAPI schema Mark stored procedure parameters without defaults as required in OpenAPI Jun 26, 2026
Copilot AI requested a review from souvikghosh04 June 26, 2026 08:24
@souvikghosh04

souvikghosh04 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

@copilot please add tests from to support the changes

Copilot AI commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please add tests from to support the changes

Added tests in StoredProcedureGeneration.ValidateRequestBodyContents covering all branches of the required-parameter logic:

  • sp1: parameters without defaults are all required.
  • sp2 (insert_book): a parameter with a config-provided default is not required.
  • sp3 (update_book_title): a parameter explicitly marked required: false is not required.

Each entity uses a distinct stored procedure so per-entity config metadata doesn't cross-contaminate (entities sharing a source share the same parameter definition). Verified all rows pass with the fix and fail when it's reverted, against a local SQL Server. (79f... latest commit)

@souvikghosh04
souvikghosh04 marked this pull request as ready for review July 7, 2026 10:32
Copilot AI review requested due to automatic review settings July 7, 2026 10:32
@aaronburtle

aaronburtle commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

This introduces a third required/default precedence rule across DAB. OpenAPI now uses Required ?? !HasConfigDefault, GraphQL and describe_entities use Required ?? true, and MCP custom tools make any parameter with a default optional, even when Required == true. This produces different contracts for Required == null/true with a config default. Please confirm the canonical policy from #3400 and centralize it in a shared or clearly named helper, with truth-table coverage for all Required × HasConfigDefault combinations.

aaronburtle
aaronburtle reviewed Jul 29, 2026
View reviewed changes
Comment thread src/Core/Services/OpenAPI/OpenApiDocumentor.cs
aaronburtle
aaronburtle reviewed Jul 29, 2026
View reviewed changes
Comment thread src/Service.Tests/OpenApiDocumentor/StoredProcedureGeneration.cs
aaronburtle
aaronburtle approved these changes Jul 29, 2026
View reviewed changes

@aaronburtle aaronburtle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go once the last comments are addressed!

@souvikghosh04

souvikghosh04 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

@copilot review the open/unresolved comments as fix what necessary and respond to each unresolved comments.

@souvikghosh04
Fix GET SP query params required field; add requestBody.Required asse…
2d432dc 
…rtions and GET query param tests

Co-authored-by: souvikghosh04 <210500244+souvikghosh04@users.noreply.github.com>

Copilot AI commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

This introduces a third required/default precedence rule across DAB. OpenAPI now uses Required ?? !HasConfigDefault, GraphQL and describe_entities use Required ?? true, and MCP custom tools make a...

The inconsistency across GraphQL, MCP, and OpenAPI is a valid concern. Centralizing the predicate into a shared helper would require touching the GraphQL schema builder and MCP tool generation, which is beyond the scope of this PR. I'd recommend tracking that refactor as a follow-up item (e.g., referencing #3400), so it gets proper truth-table coverage for all three surfaces in one dedicated PR.

@copilot review the open/unresolved comments as fix what necessary and respond to each unresolved comments.

All three open comments have been addressed in the latest commit:

  • #3672485963: AddStoredProcedureInputParameters now uses Required ?? !HasConfigDefault for GET query params + ValidateGetQueryParameters test added.
  • #3672518926: ValidateRequestBodyContents now asserts requestBody.Required; sp4 entity added to cover the all-optional case (requestBody.Required == false, empty schema required set).
  • #5115225081: Replied that centralizing the predicate across GraphQL/MCP is out of scope and should be a follow-up PR.

@souvikghosh04

souvikghosh04 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 30, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).

@souvikghosh04

souvikghosh04 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 30, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).

RubenCerna2079
RubenCerna2079 approved these changes Jul 30, 2026
View reviewed changes
Comment thread src/Service.Tests/OpenApiDocumentor/StoredProcedureGeneration.cs Outdated
@RubenCerna2079

RubenCerna2079 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Aug 3, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).

@souvikghosh04
souvikghosh04 merged commit 7341bc2 into main Aug 3, 2026
14 checks passed
@souvikghosh04
souvikghosh04 deleted the copilot/fix-openapi-required-params branch August 3, 2026 20:10
@github-project-automation github-project-automation Bot moved this from Review In Progress to Done in Data API builder Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@souvikghosh04 souvikghosh04 souvikghosh04 approved these changes
@RubenCerna2079 RubenCerna2079 RubenCerna2079 approved these changes
@aaronburtle aaronburtle aaronburtle approved these changes
@Aniruddh25 Aniruddh25 Awaiting requested review from Aniruddh25 Aniruddh25 is a code owner
@anushakolan anushakolan Awaiting requested review from anushakolan anushakolan is a code owner
@JerryNixon JerryNixon Awaiting requested review from JerryNixon JerryNixon is a code owner
@sourabh1007 sourabh1007 Awaiting requested review from sourabh1007 sourabh1007 is a code owner
@vadeveka vadeveka Awaiting requested review from vadeveka vadeveka is a code owner
@Alekhya-Polavarapu Alekhya-Polavarapu Awaiting requested review from Alekhya-Polavarapu Alekhya-Polavarapu is a code owner
@rusamant rusamant Awaiting requested review from rusamant rusamant is a code owner
@stuartpa stuartpa Awaiting requested review from stuartpa stuartpa is a code owner

Assignees

@RubenCerna2079 RubenCerna2079

@aaronburtle aaronburtle

Copilot code review Copilot

Labels

None yet

Projects

Data API builder
Status: Done

Milestone

July 2026

Development

Successfully merging this pull request may close these issues.

[Bug]: Required params not marked in OpenAPI

6 participants

@souvikghosh04 @aaronburtle @RubenCerna2079 @anushakolan